layers: on delete clear also the compressed/uncompressed cache reference #730
Conversation
|
FWIW the main branch fix is a bit more involved: #665 . |
It removes the specific layer id from the layer ids array of the r.bycompressedsum[digest] (and the same for the uncompressed map). For the record, also backporting #665 would fix the issue mentioned in containers/image#1051. |
One case, probably reachable through c/image/storage, would be using the same layer on top of different parent layers. The caller also might not be c/image/storage; e.g. the caller can not supply an ID, causing a random ID to be used, or perhaps the caller might intentionally create a different layer from the same |
That's right - I find it useful to compare it to source control systems like git, where the same patch can be applied to different branches after they've diverged. |
|
Thanks for explaining! |
|
I think this PR needs to be rebased. |
Rebased, thanks! |
Signed-off-by: zvier <[email protected]> (cherry picked from commit 5dcc8ed) Signed-off-by: Francesco Giudici <[email protected]>
|
LGTM |
| if layer.MountPoint != "" { | ||
| delete(r.bymount, layer.MountPoint) | ||
| } | ||
| r.deleteInDigestMap(id) |
There was a problem hiding this comment.
Probably outside of the scope of this PR, but is there any advantage to backport the deleteInternal() function from master where this particular function is called from there?
There was a problem hiding this comment.
Good point. Tried to look into it...
AFAICS the deleteInternal() just contains the core deletion operations once in Delete().
The main difference I can see in commit bcedb54d05 on master (which introduced the function) is that now the Load() function calls deleteInternal() in place of the full Delete() function.
This skips from the Load() the operations left in Delete(), which are:
- the checks if the layers are mounted
- the call to the
Save()function
As bcedb54d05 commit message says, this was introduced to skip the mountpoints locking in the Save() function, as locking already happens in Load().
Looking at the code in the 1.11 branch, the locking management seems quite different. In particular the Save() function doesn't perform any lock.
So, my guess is that we wouldn't benefit from the deleteInternal() function here (moreover, backporting wouldn't be trivial).
There was a problem hiding this comment.
Thanks for taking a look @fgiudici. I was afraid that once you undid one string, too much would unravel with that.
|
LGTM, merging. |
6 participants
The storage cache is not completely cleaned when removing images.
Remove also the entries referencing layers by compressed and uncompressed sums.